feat(multiple_inherent_impl): Add config option to target specific scope #15843

paulmialane · 2025-10-08T09:22:32Z

Add a config option inherent-impl-lint-scope to the lint multiple_inherent_impl to target a different scope according to people's needs. It can take three values: module, file, and crate (default).

module is the weakest option. It lints if there are two or more impls in the same module.
file is a bit stronger, since it lints if there are two or more impls in the same file. So, this triggers the lint (where it did not with module):
crate is the strongest of them; it triggers as soon as there are two or more impls anywhere in the crate. It is the current behaviour of the lint, so it's the default option.

changelog: [multiple_inherent_impl] : Add config option (module, file or crate) to target specific scope

fixes #14867

rustbot · 2025-10-08T09:22:38Z

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

samueltardieu

Thanks for your contribution. There are some things that need to be changed in the way the configuration option is parsed (and output in messages) so that lower-case versions "crate", "file", and "module" can be used instead of a mixed-case one.

View changes since this review

clippy_config/src/types.rs

samueltardieu · 2025-10-08T10:21:13Z

clippy_lints/src/inherent_impl.rs

    ///
+    /// The config option controls the scope at which multiple inherent impl blocks for the same
+    /// struct are linted, allowing values of "module" (only within the same module), "file"
+    /// (within the same file), or "crate" (anywhere in the crate, default).


Those lowercase values are not currently recognized (yet), see other comment.

samueltardieu · 2025-10-08T10:47:01Z

tests/ui-cargo/multiple_inherent_impl/crate_fail/src/main.rs

+struct T;
+
+impl S {
+    fn fisrt() {}


Typo? (several occurrences)

samueltardieu

Thanks for the update. Here are a new round of comments, the PR is starting to look good.

View changes since this review

samueltardieu · 2025-10-08T15:23:39Z

clippy_config/src/conf.rs

    /// A list of paths to types that should be treated as if they do not contain interior mutability
    #[lints(borrow_interior_mutable_const, declare_interior_mutable_const, ifs_same_cond, mutable_key_type)]
    ignore_interior_mutability: Vec<String> = Vec::from(["bytes::Bytes".into()]),
+    /// Sets the scope at which duplicate impl blocks for the same type are linted.


Nit

Suggested change

/// Sets the scope at which duplicate impl blocks for the same type are linted.

/// Sets the scope ("crate", "file", or "module") in which duplicate inherent `impl` blocks for the same type are linted.

samueltardieu · 2025-10-08T15:42:38Z

clippy_lints/src/inherent_impl.rs

                    Entry::Vacant(e) => {
                        // Store the id for the first impl block of this type. The span is retrieved lazily.
-                        e.insert(IdOrSpan::Id(impl_id));
+                        e.insert((IdOrSpan::Id(impl_id), criterion.clone()));


There seems to be no need to insert the criterion as part of the value (while it is useful as part of the key). Can't you just keep inserting the IdOrSpan as was done before? Also, this would let you remove the .clone() and write match type_map.entry((impl_ty, criterion)) { above.

samueltardieu · 2025-10-08T15:43:24Z

clippy_lints/src/inherent_impl.rs

                    Entry::Occupied(mut e) => {
                        if let Some(span) = get_impl_span(cx, impl_id) {
-                            let first_span = match *e.get() {
+                            let first_span = match e.get().0 {


You can also restore the dereference once you remove the criterion.

samueltardieu

Could you also please squash the commits?

View changes since this review

samueltardieu · 2025-10-09T08:14:25Z

clippy_lints/src/inherent_impl.rs

+                        if let Node::Item(&Item {
+                            kind: ItemKind::Impl(_impl_id),
+                            span,
+                            ..
+                        }) = cx.tcx.hir_node(hir_id)
+                        {
+                            Criterion::File(cx.tcx.sess.source_map().lookup_source_file(span.lo()).name.clone())
+                        } else {
+                            // We know we are working on an impl, so the pattern matching can
+                            // not fail
+                            unreachable!()
+                        }


Suggested change

if let Node::Item(&Item {

kind: ItemKind::Impl(_impl_id),

span,

..

}) = cx.tcx.hir_node(hir_id)

{

Criterion::File(cx.tcx.sess.source_map().lookup_source_file(span.lo()).name.clone())

} else {

// We know we are working on an impl, so the pattern matching can

// not fail

unreachable!()

}

let span = cx.tcx.hir_span(hir_id);

Criterion::File(cx.tcx.sess.source_map().lookup_source_file(span.lo()).name.clone())

samueltardieu · 2025-10-09T08:20:38Z

clippy_lints/src/inherent_impl.rs

            || is_lint_allowed(
                cx,
                MULTIPLE_INHERENT_IMPL,
                cx.tcx.local_def_id_to_hir_id(id),


I think this will not work if #[expect] is used on the type definition, as the expectation will not be fulfilled. It would probably be better to call clippy_utils::fulfill_or_allowed(…). This must be done only when you know for sure that the lint would be emitted if not allowed or expected.

Could you add some tests with #[expect] to check that this works as expected (ah ah)?

paulmialane · 2025-10-09T11:45:28Z

I need help to understand what's wrong with my test file.

I looked at how #[expect(...)] works, and as suggested I tried using fulfill_or_allowed(). I think I have the desired behaviour, as when I test on my own crate using cargo dev lint ... I have the expected errors.

But, when I use cargo uibless on my new test file:

error: diagnostic code `clippy::unfulfilled_lint_expectations` not found on line 80
  --> tests/ui/impl.rs:81:6
   |
81 | //~^ unfulfilled_lint_expectations
   |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected because of this pattern
   |

error: there were 1 unmatched diagnostics
  --> tests/ui/impl.rs:80:10
   |
80 | #[expect(clippy::multiple_inherent_impl)]
   |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Error[unfulfilled_lint_expectations]: this lint expectation is unfulfilled

I don't understand why I get this... The way I wrote it is probably not the intended way, but then how?

Thank you!

samueltardieu

Looks like only a minor comment change is necessary to ensure consistency.

I also find the IdOrSpan construct a bit useless, but that is not on you as this is already present. I have plans to remove this later once your PR is merged, and do some cleanup in the way this lint works, unrelated to your changes.

View changes since this review

samueltardieu · 2025-10-10T17:47:46Z

clippy_lints/src/inherent_impl.rs


        for (&id, impl_ids) in &impls.inherent_impls {
            if impl_ids.len() < 2
            // Check for `#[allow]` on the type definition


Suggested change

// Check for `#[allow]` on the type definition

// Check for `#[expect]` or `#[allow]` on the type definition

rustbot assigned samueltardieu Oct 8, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 8, 2025

samueltardieu requested changes Oct 8, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 8, 2025

paulmialane requested a review from samueltardieu October 8, 2025 12:58

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 8, 2025

samueltardieu requested changes Oct 8, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 8, 2025

paulmialane requested a review from samueltardieu October 9, 2025 07:34

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 9, 2025

samueltardieu requested changes Oct 9, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 9, 2025

feat(multiple_inherent_impl): Add config option to target specific scope

97de6e0

paulmialane force-pushed the feat/multiple_inherent_impl branch from 21e0f33 to 97de6e0 Compare October 10, 2025 16:53

paulmialane requested a review from samueltardieu October 10, 2025 17:40

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Oct 10, 2025

samueltardieu requested changes Oct 10, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Oct 10, 2025

	/// Sets the scope at which duplicate impl blocks for the same type are linted.
	/// Sets the scope ("crate", "file", or "module") in which duplicate inherent `impl` blocks for the same type are linted.

	// Check for `#[allow]` on the type definition
	// Check for `#[expect]` or `#[allow]` on the type definition

feat(multiple_inherent_impl): Add config option to target specific scope #15843

Are you sure you want to change the base?

feat(multiple_inherent_impl): Add config option to target specific scope #15843

Uh oh!

Conversation

paulmialane commented Oct 8, 2025 • edited by samueltardieu Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Oct 8, 2025

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulmialane commented Oct 9, 2025

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

paulmialane commented Oct 8, 2025 •

edited by samueltardieu

Loading

samueltardieu left a comment •

edited by rustbot

Loading

samueltardieu left a comment •

edited by rustbot

Loading

samueltardieu left a comment •

edited by rustbot

Loading

samueltardieu left a comment •

edited by rustbot

Loading